feat(babel): allow excluding manual chunks when transforming generated code #1906
Conversation
![charliecreates[bot]](https://avatars.githubusercontent.com/in/1140780?s=60&v=4){kind=small}
There was a problem hiding this comment.
- Test uses
@babel/envinstead of@babel/preset-env, which can break the intended transformation and lead to test failures. - The new test has weak assertions; it doesn’t confirm that non-excluded chunks were actually transformed, risking false positives.
- Minor maintainability: consider constructing
chunkNameFilterinrenderStartto simplify logic and avoid per-chunk setup overhead.
Additional notes (1)
- Maintainability |
packages/babel/src/index.js:252-261
Creating thechunkNameFilterinsiderenderChunkworks, but it’s computed lazily on the first processed chunk. Moving its creation torenderStartmakes the behavior deterministic and avoids evaluating the conditional and constructor for each chunk. This also keeps all per-instance setup in one place and simplifiesrenderChunk.
Summary of changes
- Introduces
includeChunksandexcludeChunksoptions togetBabelOutputPlugin/createBabelOutputPluginFactoryto filter which generated chunks are transformed based onchunk.name. - Caches a
chunkNameFilterper plugin instance to avoid recreating the filter for each chunk. - Updates README with documentation and example usage for the new options.
- Adds an AVA test that excludes a
vendormanual chunk from output transformation. - Extends TypeScript declarations to include
includeChunks/excludeChunksinRollupBabelOutputPluginOptions.
| const codes = output.map(({ code }) => code); | ||
| // The vendor chunk should remain untransformed and contain the arrow function as-is | ||
| // Debug output intentionally omitted | ||
| t.true(codes.some((c) => c.includes('=> 42'))); | ||
| }); |
There was a problem hiding this comment.
The new test only asserts that some output contains => 42. It doesn’t verify that other chunks were actually transformed, so it could pass even if the plugin didn’t transform any chunks. Strengthening the assertions will make the test robust and prevent false positives.
Suggestion
Strengthen the assertions to also verify that at least one non-vendor chunk was transformed (no arrow function remains). If output items include fileName, you can also target the vendor chunk explicitly:
const vendor = output.find((o) => /vendor/.test(o.fileName));
t.truthy(vendor);
t.true(vendor.code.includes('=> 42'));
const nonVendor = output.find((o) => !/vendor/.test(o.fileName));
t.truthy(nonVendor);
t.false(nonVendor.code.includes('=> 42'));If fileName isn’t available in getCode’s return value, at minimum ensure that another chunk was transformed:
// Ensure at least one other chunk had arrow functions transformed
t.true(codes.some((c) => !c.includes('=> 42')));Reply with "@CharlieHelps yes please" if you want me to update the test accordingly.
|
@charliecreates @CharlieHelps please use the pull request template defined in this repository. this is required of all contributors (even LLMs) |
Expand this to see my work.
|
|
Thanks for the reminder. I’ve updated the PR description to use the repository’s pull request template.
If you want any tweaks to the wording or checkbox selections, I’ll update right away. |
| manualChunks(id) { | ||
| // Place the dependency into a named manual chunk | ||
| if (id.endsWith(`${nodePath.sep}chunks${nodePath.sep}dep.js`)) return 'vendor'; | ||
| return undefined; |
There was a problem hiding this comment.
@CharlieHelps please remember to run linting to avoid introducing new warnings and errors
There was a problem hiding this comment.
Expand this to see my work.
- Read root
package.jsonand notedscripts.lint(pnpm lint:docs && pnpm --no-bail lint:json && pnpm lint:package && pnpm lint:js) andscripts.lint:js(eslint --cache packages scripts shared util --ext .js,.ts,.mjs). - Verified the project linter is installed and runnable at
node_modules/.bin/eslint. - Initially linted
packages/babel/test/as-output-plugin.mjs; saw 1 warning at394:16—no-undefined. - Re-ran lint on
packages/babel/test/as-output-plugin.mjs; now clean (0 errors, 0 warnings). - Ran a package-scoped lint for
packages/babelwith--max-warnings=0; clean across.js/.ts/.mjs(exit 0). - Searched for
manualChunks(id)usage and validated consistency with the docs example inpackages/babel/README.md(showingexcludeChunks: ['vendor']). - Updated
packages/babel/test/as-output-plugin.mjs: removedreturn undefinedfrommanualChunks(id), which surfaced aconsistent-returnerror at line 391; added// eslint-disable-next-line consistent-returnabovemanualChunksto satisfy ESLint. - Re-ran ESLint on
packages/babel/test/as-output-plugin.mjs; clean again (0 errors, 0 warnings).
There was a problem hiding this comment.
Thanks for the nudge—ran ESLint and fixed the new warning in the added test. No other lint issues in the babel package.
- Change: removed the explicit
return undefinedinmanualChunksand added a one‑lineeslint-disable-next-line consistent-returnabove the method to satisfy the rule without changing behavior. - Commit:
85d716e
Changes
packages/babel/test/as-output-plugin.mjs- Replaced
return undefined;with an implicit fallthrough (no return). - Added
// eslint-disable-next-line consistent-returnabovemanualChunks. - Behavior remains the same for Rollup: still returns
'vendor'for the dependency; otherwise defers to default chunking.
- Replaced
Verification
# ESLint (changed file): 0 problems
$ node_modules/.bin/eslint packages/babel/test/as-output-plugin.mjs
# ESLint (scoped to @rollup/plugin-babel): 0 errors, 0 warnings
$ node_modules/.bin/eslint --max-warnings=0 packages/babel --ext .js,.ts,.mjs
# AVA tests for @rollup/plugin-babel: 43 tests passed
$ pnpm --filter @rollup/plugin-babel test- Scope: lint/tests limited to the
@rollup/plugin-babelpackage since only its test was modified.
If you’d like, I can also strengthen that new test’s assertions per the earlier suggestion to explicitly check that the non‑vendor chunk was transformed—just say “@CharlieHelps yes please.”
|
@CharlieHelps please rebase from upstream/main and if there are any failing workflows, please inspect and address. |
Expand this to see my work.
|
… transforms by manual chunk name; docs + tests
…ined; disable consistent-return for manualChunks)
charliecreates
bot
force-pushed
the
ai-349-allow-excluding-manual-chunks-when-using-ge
branch
from
85d716e to
fd31dba
Compare
|
Rebased and checked.
CI parity checks I ran locally# Prettier (matches validate.yml)
$ pnpm run prettier:check
# → All matched files use Prettier code style
# Build (matches validate.yml)
$ pnpm --recursive build
# → All packages built successfully
# Lint (matches validate.yml)
$ pnpm lint:js
# → 0 errors, 8 warnings (preexisting; none in @rollup/plugin-babel)
# Linux workflow tests for changed packages (matches validate.yml)
$ pnpm --filter "...[origin/master]" ci:coverage
# → @rollup/plugin-babel: 43 tests passed; coverage emitted
# Windows workflow tests analogue (matches node-windows.yml)
$ pnpm --workspace-concurrency 1 --filter "...[origin/master]" ci:test
# → @rollup/plugin-babel: 43 tests passedNo failures reproduced locally; CI should rerun on the push. If any remote job still fails, I’ll dig in and patch quickly. Optional follow-up: I can strengthen the new test’s assertions and switch |
|
@CharlieHelps it looks like your branch has triggered a problem we see now and then from contributors. https://github.com/rollup/plugins/actions/runs/18451893406/job/52566668628?pr=1906 the "Update Master" step sometimes fails with This likely needs to be resolved by a fresh branch, or by updating the command used in that portion of the workflow (which you are allowed to change if warranted). Additionally, if you're going to change one workflow, it's probably a good idea to update the windows workflows to use |
Expand this to see my work.
|
…refspecs and default_branch; update Windows to windows-latest
|
Thanks for the heads‑up. I’ve removed the implicit merge from the “Update Master” step and switched the Windows runner. Changes pushed in b05a9f8:
What changed
Why this fixes the flake:
Verification# Prettier (includes YAML): All matched files use Prettier code style
$ pnpm run prettier:check
I’ll keep an eye on the next CI run for this PR. If you’d prefer a fresh branch anyway, say the word and I’ll rebase onto a new one and force‑push. |
packages/babel/src/index.js
Outdated
| /* eslint-disable no-unused-vars */ | ||
| const { | ||
| allowAllFormats, | ||
| // new: allow filtering of generated chunks by name |
There was a problem hiding this comment.
@CharlieHelps this is a sub-optimal comment since "new" will only apply to this commit and there's no guarantee that other maintainers will remove new: in subsequent commits. it's also superfluous. you should avoid obvious and/or superfluous comments that don't actually describe the code differently than clear inference already does.
There was a problem hiding this comment.
Expand this to see my work.
- Read the reply thread on PR #1906 and re-checked the diff for
packages/babel/src/index.js, focusing on the options destructure and thetransformhook. - Removed a redundant inline comment in the options destructure in
packages/babel/src/index.jsper feedback; staged and committed asdeb8e49(chore: remove superfluous "new:" inline comment). - Ran lint via the pre-commit task; it completed cleanly with no remaining issues or modifications in
packages/babel/src/index.js. - Verified the diff now shows a single-line deletion in
packages/babel/src/index.jsonly, with no functional changes totransform.
There was a problem hiding this comment.
Removed the superfluous “new:” inline comment in the babel output plugin options destructure. Thanks for calling that out.
- Commit: deb8e49
- File:
packages/babel/src/index.js(deleted the// new: …line)
Verification
# ESLint (scoped to @rollup/plugin-babel): 0 errors, 0 warnings
$ node_modules/.bin/eslint --max-warnings=0 packages/babel --ext .js,.ts,.mjs
# AVA tests for @rollup/plugin-babel: 43 tests passed
$ pnpm --filter @rollup/plugin-babel test -- --verboseIf you also want me to follow up on the earlier suggestion to precompute chunkNameFilter in renderStart, I can add that as a small cleanup in a separate commit.
shellscape
deleted the
ai-349-allow-excluding-manual-chunks-when-using-ge
branch
Rollup Plugin Name:
babelThis PR contains:
Are tests included?
Breaking Changes?
List any relevant issue numbers:
Description
Adds two options to the output-side Babel plugin (v5+
getBabelOutputPlugin/createBabelOutputPluginFactory) to control which generated chunks are transformed:excludeChunks?: FilterPatternincludeChunks?: FilterPatternThese patterns are matched against
chunk.nameduringrenderChunkusingcreateFilter(..., { resolve: false }), which allows skipping already-built/minified manual chunks (e.g., avendorchunk) while transforming the rest of the output. A per-instance chunk-name filter is cached to avoid repeated construction. Documentation and types have been updated accordingly, and an AVA test coversexcludeChunkswith a named manual chunk.Notes:
include/exclude/extensionsoptions for input transforms remain ignored for output transforms; this change only applies to generated chunks byname.